no unnecessary string modifications #543
Conversation
Because crskin.cpp & wolutil.cpp did use some of the stuff you removed? |
| @@ -617,12 +617,13 @@ lString32 MathMLHelper::getMathMLAdjustedText(ldomNode * node, const lChar32 * t | |||
| // (no substitution for "normal") | |||
| if ( mathvariant != mathml_mathvariant_normal ) { | |||
| len = mtext.length(); | |||
| lChar32 *ptr = mtext.modify(); | |||
There was a problem hiding this comment.
Feels a bit less object oriented, if we need to get to the raw char array to modify the string.
Any idea for a new lStringXY method(s) to keep it opaque ? (ie mtext.replaceCharAt(i, c))
Was I really the only user of this feature, in mathml.cpp ?
There was a problem hiding this comment.
I prefer an explicit access myself. Plus it means you don't have to go through a call to modify() for each character.
There's one use in there, yes. |
There was a problem hiding this comment.
Fine.
I would squash these 4 commits as a single one, as they end up being dependant:
Avoid unnecessary string modifications
See https://github.com/koreader/crengine/pull/543:
(+ the text of the 4 commits we'll get with Squash)
But waiting for you to unmark Draft if you think this is ready.
Why? There's nothing to be gained, you loose context in the subject line (e.g. in blame), end-up with not strictly related changes in the commit. Generally speaking: more (simpler) commits = better, no? |
For the review, yes indeed! Thank you for that. If you really want to keep the individual commits, please add a common prefix |
See koreader#543: - lvstring: drop `& operator []` accessors, as they end up always being used, even for read-only lookup, triggering unnecessary calls to `modify()` strings. - avoid unnecessary use of `lStringXX:at(…)` (which could trigger a call to `modify()`). - lvstring: fix `lastChar/firstChar` implementations: don't use `at()`, which could trigger a call to `modify()`. - lvstring: drop `at()` methods: not used anymore.
benoit-pierre
force-pushed
the
pr/no_unnecessary_string_modifications
branch
from
b2e3ddc to
60506b8
Compare
|
I disagree, but OK, your project, your rules. |
|
No... you shouldn't have done this yourself. We should have kept your 4 commits in this PR, for reference. (See @NiLuJe 's PRs :) Now, we can't see them anywhere, and the above discussion has lost infos and context :( |
benoit-pierre
force-pushed
the
pr/no_unnecessary_string_modifications
branch
from
60506b8 to
082a9f0
Compare
|
It's already annoying to not be able to use |
|
I understand. (You still need to remove the Draft state when you want to proceed.) |
I'll remove it once koreader/koreader-base#1675 and its dependent are merged. |
Also Draft. |
That one can wait (this way I'll cleanup the no longer necessary |
As they end up always being used, even for read-only lookup, triggering unnecessary calls to `modify()` strings.
Which could trigger a `modify()`.
Don't use `at()`, which could trigger a call to `modify()`.
Not used anymore.
benoit-pierre
force-pushed
the
pr/no_unnecessary_string_modifications
branch
from
082a9f0 to
0b0c536
Compare
The extra
value_type & operator []accessors would get picked-up over the normal readonly one:value_type operator [], resulting in unnecessary calls tomodify()(triggering an allocation+copy if the reference count is not 1). Get rid of the former, and fixlastCharandfirstCharmethod to also be read-only. Additionally, avoid using theat()method (prefer an explicitmodify()[…]when needed), and remove it.Note: depends on koreader/koreader-base#1675.
This change is